Skip to content

CQL Test Server for CQL Tests Runner#360

Merged
cmoesel merged 11 commits into
masterfrom
test-server
May 27, 2026
Merged

CQL Test Server for CQL Tests Runner#360
cmoesel merged 11 commits into
masterfrom
test-server

Conversation

@cmoesel
Copy link
Copy Markdown
Member

@cmoesel cmoesel commented Apr 16, 2026

This PR implements a test server that exposes a $cql target to allow the CQL Tests Runner to run tests against the cql-execution engine. Rather than repeat all the details here, I'll refer you to the test-server/README.md file in this branch.

I initially developed this as a standalone project outside of cql-execution that pulled in cql-execution as an NPM dependency. This PR integrates it into the cql-execution project as a sub-project with its own package.json and configuration files. In addition to general review of the code, I'm also interested in thoughts on this approach. Should we:

  • Keep it as I have it here (a sub-project)? UPDATE: We chose this approach.
  • Keep it in cql-execution but try to integrate it more closely (i.e., share package.json and configuration files)?
  • Or introduce it as a completely separate project outside of cql-execution (and if so, where)?

I have not yet invested in updating GitHub actions to automatically run the tests or other checks in the sub-project. I will do that after we have confirmed the approach. UPDATE: GitHub Actions are now in place.

NOTE: The npm audit check is currently failing due to a reported vulnerability in elliptic. There is currently no fix for that.

Submitter:

  • This pull request describes why these changes were made
  • Code diff has been done and been reviewed (it does not contain: additional white space, not applicable code changes, debug statements, etc.)
  • Tests are included and test edge cases
  • Tests have been run locally and pass
  • Code coverage has not gone down and all code touched or added is covered.
  • Code passes lint and prettier (hint: use npm run test:plus to run tests, lint, and prettier)
  • All dependent libraries are appropriately updated or have a corresponding PR related to this change
  • [n/a] cql4browsers.js built with npm run build:browserify if source changed.

Reviewer:

Name:

  • Code is maintainable and reusable, reuses existing code and infrastructure where appropriate, and accomplishes the task’s purpose
  • The tests appropriately test the new code, including edge cases
  • You have tried to break the code

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.58%. Comparing base (d1b1b14) to head (ba69a40).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #360   +/-   ##
=======================================
  Coverage   87.58%   87.58%           
=======================================
  Files          52       52           
  Lines        4606     4606           
  Branches     1297     1297           
=======================================
  Hits         4034     4034           
  Misses        359      359           
  Partials      213      213           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cmoesel added 5 commits May 11, 2026 14:39
- fully self-contained project as subfolder
- uses release version of cql-execution
Ideally we would use the code from src directly, but typescript isn't happy
- Use NodeNext module resolution
- Specify mocha types in test config
Copy link
Copy Markdown
Contributor

@hossenlopp hossenlopp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach makes sense. The embedded project makes it always available for running.

I'm not 100% sure if it is universally using cql-execution in the parent directory when cql-exec-fhir is being used. Looking in the package-lock.json shows it is referencing the version on npm. This could cause some oddities with the PatientSource created by cql-exec-fhir, which may be using the npm version instead of the local.

I would try setting up cql-execution as a dependency to ../ and using cql-execution "normally", this hopefully will make sure cql-exec-fhir and your runner code are using the same cql-execution.

@elsaperelli elsaperelli self-requested a review May 13, 2026 17:05
@elsaperelli elsaperelli self-assigned this May 13, 2026
Copy link
Copy Markdown
Contributor

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!!! Posting some of the review comments I have so far!

Comment thread test-server/README.md Outdated
Comment thread test-server/README.md Outdated
Comment thread test-server/README.md
Comment thread test-server/README.md
Comment thread test-server/README.md
Comment thread test-server/src/operation.ts Outdated
cmoesel and others added 2 commits May 13, 2026 17:17
Co-authored-by: Elsa Perelli <eperelli@mitre.org>
- Force tsx watch to watch all ts files
- Ensure .env is loaded before any other code or imports
- Accept Content-Type: application/fhir+json
- Add cqf-cqlType extension to example response in README
@cmoesel
Copy link
Copy Markdown
Member Author

cmoesel commented May 14, 2026

Thanks for the excellent review comments, @elsaperelli. I've accepted the typo suggestions in b5f0daa and implemented fixes for the others in f92c40d. Thanks for keeping me honest. I guess I was rushing to get things done before my vacation more than I realized!

@cmoesel
Copy link
Copy Markdown
Member Author

cmoesel commented May 14, 2026

@hossenlopp - Good point regarding cql-exec-fhir. I thought that cql-execution was declared as a peer dependency on that so it would just use whichever one is already in the dependencies, but I never confirmed via the package-lock.json. I'll take a look at that.

IIRC, I did try to do a "normal" import via ../ first but I just could not get it to work with the TypeScript compiler. Perhaps I will give it another go, because I really would prefer that it work that way anyway.

The test-server does not need to support the FHIR model, as all tests are model-agnostic.
@cmoesel
Copy link
Copy Markdown
Member Author

cmoesel commented May 14, 2026

@hossenlopp - Thanks for noticing the issue with cql-exec-fhir. TBH, I misunderstood your comment at first (see above), but I understand now. As it turns out, the test-server doesn't need to use cql-exec-fhir at all because the tests are data-model-agnostic. 6c30420 removes the cql-exec-fhir dependency and uses the simpler PatientSource from cql-execution instead. This resulted in a very happy result: way-improved performance! The test runner used to take about 1m 25s to run. Now it takes about 10s to run! Woah!

I also investigated what it would take to have the test-server rely on the cql-execution source .ts files directly (instead of relying on compiled source in lib). This was quite tricky, but Codex eventually figured it out. By importing the ts files directly, the test-server can watch the cql-execution source for changes and hot-reload them. That's pretty cool, but it comes at a cost: the configuration is a bit more complex. I'm not sure if hot-loading cql-execution from the test-server is worth the additional complexity or not. For now I put it in a separate branch. Please take a look at this commit and let me know if you think we should adopt it or leave things as-is (you too, @elsaperelli). If we leave things as-is, it means you need to manually stop and restart the test server if you make any changes in cql-execution source.

@hossenlopp
Copy link
Copy Markdown
Contributor

@cmoesel Replying to your work on getting the hot-reload setup. It looks like that approach could be slightly fragile. I don't make use of the watch re-run features. I feel like if I was iterating on a fix, it makes more sense to be testing on the tests in the core package instead of the cql-test-server based tests.

Comment thread tsconfig.json
Copy link
Copy Markdown
Contributor

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment, but I am unsure of it so I will go ahead and approve! Woohoo!

Comment thread test-server/src/convert/convert.ts
@cmoesel
Copy link
Copy Markdown
Member Author

cmoesel commented May 27, 2026

I've addressed @hossenlopp's comments, but since he is currently unavailable to re-review, I will merge with @elsaperelli's recent approval. Thanks @hossenlopp and @elsaperelli!

@cmoesel
Copy link
Copy Markdown
Member Author

cmoesel commented May 27, 2026

Oh wait! I forgot to add GitHub actions! I will do that first.

@cmoesel cmoesel merged commit 5bc96b7 into master May 27, 2026
12 of 14 checks passed
@cmoesel cmoesel deleted the test-server branch May 27, 2026 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants